Conversation
Replace the per-event deque/linear-scan property cache with a two-level scheme: a persistent name-to-index map in schema_locator (built once per event type, shared across all events) and a flat vector<property_info> in parser indexed by property position. This eliminates per-event hash table allocation and removes string comparisons during the blob walk. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace const std::wstring& with std::wstring_view in parser's public API (parse, try_parse, view_of) and internal helpers (find_property, assert_valid_assignment, throw_if_invalid). This eliminates heap-allocating std::wstring temporaries when callers pass string literals (the common case). Property names longer than MSVC's SSO threshold of 7 wchar_t previously caused a heap allocation and free per field access. Not a breaking change - std::wstring_view is implicitly constructible from both std::wstring and const wchar_t*. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
kylereedmsft
left a comment
kylereedmsft
left a comment
There was a problem hiding this comment.
Thank you for the PR. I know this has already been merged, but this change raises some new ideas that we should consider.
| * Returns nullptr if pSchema is null or not in the cache. | ||
| * </summary> | ||
| */ | ||
| const property_name_map* get_property_names(const TRACE_EVENT_INFO* pSchema) const; |
There was a problem hiding this comment.
This doesn't seem like the right place for this. schema_locator shouldn't be aware of names. That's a responsibility for the schema or maybe the parser.
| * Keys are wstring_views pointing into stable TRACE_EVENT_INFO memory. | ||
| * </summary> | ||
| */ | ||
| using property_name_map = std::unordered_map<std::wstring_view, ULONG>; |
There was a problem hiding this comment.
Maybe we should just store the name->property_info directly. This still feels like something that should be on the schema instead of in the locator. Maybe on the trace_context?
|
|
||
| // Maintain a mapping from property name to blob data index. | ||
| std::deque<std::pair<const wchar_t *, property_info>> propertyCache_; | ||
| // Persistent name to index map shared across all events of the same type. |
There was a problem hiding this comment.
I'm not sure this layer of caching even makes sense. It's just overhead.
I can't think of any reason not to just maintain a map of name->property_info from the schema.
I'm fuzzy on all the various event type schemas, but looking at this code, it seems like the property_info blobs are expected to be consistent. Just trying to think through whether we ever need any of the old fallback code if we pre-populate and cache.
|
Thanks @kylereedmsft for the review - @jdu2600 if you have a moment, would you take a look? |
3 participants
I'm writing a modern Rust ETW parsing library, and was doing some parsing benchmarking.
I was surprised at just how more performant my approach was againt existing approaches so I did a little digging.
The main issue for krabs seemed to be a linear scan over a per-event property cache. This is effectively an O(N2) scan if you grab all properties - and the cache actually only hits if you grab the same property twice. 😱
Plus wstring temporaries.